sys/log: add format attribute to modlog_printf prototype #3530
Conversation
|
Note that there is pre-existing usage of |
ehaas
force-pushed
the
modlog-format-attribute
branch
3 times, most recently
from
1e70331 to
acedf5a
Compare
|
@ehaas I think once the nimble PR is merged, the CI issues will get fixed. Let's wait for that. |
sjanc
left a comment
sjanc
left a comment
There was a problem hiding this comment.
what setup is giving you those errors where int32_t is long int? we could add this to CI to avoid similar issues in future
net/oic/src/port/mynewt/ip_adaptor.c
Outdated
| rc = mn_sendto(oc_ucast6, n, (struct mn_sockaddr *) &to); | ||
| if (rc != 0) { | ||
| OC_LOG_ERROR("Failed to send buffer %u on itf %d\n", | ||
| OC_LOG_ERROR("Failed to send buffer %u on itf %" PRIu32 "\n", |
There was a problem hiding this comment.
I'm really not a huge fan of PRI syntax... especially that it leads to inconsistent print formatting, eg here you don't use PRIu16 for omp_len
what do you think on using %u here and casting msin6_scope_id to (unsigned int) ?
There was a problem hiding this comment.
Updated; I also find the PRI syntax pretty ugly.
| */ | ||
| void modlog_printf(uint8_t module, uint8_t level, const char *msg, ...); | ||
| void modlog_printf(uint8_t module, uint8_t level, const char *msg, ...) | ||
| __attribute((format(printf, 3, 4))); |
There was a problem hiding this comment.
modlog.h is included in a lot of modules in core, only osdp and oic require fixes for this to be enabled?
maybe it should be under MYNEWT_VAL (at least for time being) so that no unexpected build errors are introduced?
(especially that currently we always enable Werror - which we should make opt-in option anyway)
yeah, I guess any reasonable compiler supports this extension :) |
Here is a repro on arm-none-eabi-gcc 8.3.1: https://godbolt.org/z/sTnPahqE5 |
ehaas
force-pushed
the
modlog-format-attribute
branch
from
acedf5a to
de2fb82
Compare
sjanc
left a comment
sjanc
left a comment
There was a problem hiding this comment.
looks ok to me but please fix compliance issues reported
I guess Nimble PR also should be updated to use cast instead PRI and this should be ready for merging
|
@ehaas commit body is required by mynewt, can you please update the commit with the body. |
Add format(printf) attribute to modlog_printf to enable compile-time checking that format strings match arguments. By default the attribute is disabled; it can be enabled by setting MODLOG_USE_PRINTF_ATTRIBUTE to 1. Note that enabling the attribute will cause incorrect modlog_printf usage which previously compiled successfully, to no longer compile.
ehaas
force-pushed
the
modlog-format-attribute
branch
from
b2c390b to
1764a28
Compare
vrahane
left a comment
vrahane
left a comment
There was a problem hiding this comment.
Since all comments have been addressed, I think we are good to merge this in.
3 participants
Add format(printf) attribute to modlog_printf to enable compile-time checking that format strings match arguments.
Note that this will cause incorrect modlog_printf usage which previously compiled successfully, to no longer compile.